Oh and it seems like you need to rebase.

Hey @evanlinjin and @LLFourn, thanks a lot for your reviews. I've given some thoughts about them:

1. I agree that having a BTreeMap with everything inside is not optimal, and a structure with both last_revealed and descriptor would be better. That being said, we need to make sure that users don't pass in a (keychain, index) without the correspondent descriptor.
2. The more I think about it, the more I convince myself that we shouldn't let the user modify the descriptor - otherwise, what's the point of having a db checksum? I think that changing it silently (as my code is doing right now), or making the changeset monotone as lloyd suggested, might be potential footguns. (I might not have fully comprehended lloyd's proposal though!)

It seems to me that it would be optimal to have append return a Result, and error in the two cases; but the Append trait doesn't specify any return value for append. How should I go about this?

Just quoting @LLFourn's response to adding an error to Append trait methods:

Because we should design things so that there should be no possible error

The errors would have to be programmer errors which should be handled via panic or just carry on with best effort.

I think there would most of the time be ways to re-architect the changeset to avoid errors completely. That's the trade off. Either design it so there can't be errors (may mean bigger more monotone changesets) or just accept the error.

What do we think about using debug_assert! in the Append implementation for now? This will check if we are mutating the descriptor of an already-existing keychain. In release mode, the consecutive changes to a keychain's descriptor should just be ignored.

The add_keychain method should error if a user is trying to modify an already-existing keychain. The changesets are returned by KeychainTxOutIndex methods. The KeychainTxOutIndex should never create consecutive changesets that modify the same keychain's descriptor. So I think my suggestion is fair.

I've been trying to think of how to keep ChangeSet<K> monotone and I think we should not change the keychain::ChangeSet and instead enforce that descriptors don't change at the bdk::wallet::ChangeSet level. This puts responsibility on the devs who who don't use bdk::wallet to keep their data consistent. They can either follow how we do it in Wallet or go their own way if they know what they're doing. I created #1234 to demonstrate this approach, would love some feed back.

This is actually becoming my intuition too but have an idea a bit different #1234. I think the descriptor has to be in the keychain changeset because that's where it's stored in memory and it has to be there to produce new addresses. Having it elsewhere adds friction and removing that is the point of this effort.

We can keep KeychainTxOutIndex simple and keep changesets montone by removing the type parameter K from it. There is a perfectly coherent bit of functionality with its own persistence mechanism that cannot conflict with anything else that we can carve out here and leave labeling to later. For now we can let Wallet handle its own labeling with the KeychainKind enum in its own addition to its changeset.

So here's my suggestion:

1. Use descriptor hash to identify descriptor (as @notmandatory does in his approach). Let's call it DescriptorId and make it type DescriptorId = sha256::Hash for now.
2. Rename KeychainTxOutIndex to DescriptorTxOutIndex
3. Define it as follows:

#[derive(Clone, Debug)]
pub struct DescriptorTxOutIndex {
    inner: SpkTxOutIndex<(DescriptorId, u32)>,
    // descriptors of each keychain
    descriptors: BTreeMap<DescriptorId, Descriptor<DescriptorPublicKey>>,
    // last revealed indexes
    last_revealed: BTreeMap<DescriptorId, u32>,
    // lookahead settings for each descriptor
    lookahead: BTreeMap<DescriptorId, u32>,
}

#[derive(Clone, Debug, PartialEq, serde::Deserialize, serde::Serialize)]
pub struct ChangeSet {
    revealed: BTreeMap<DescriptorId, u32>
    added: BTreeSet<Descriptor<DescriptorPublicKey>
}

Methods in DescriptorTxOutIndex should take a DescriptorId.

This changeset is monotone and error free and gives us most of the useful things that KeychainTxOutIndex did. We could of course keep KeychainTxOutIndex and implement with a DescriptorTxOutIndex internally but I think we should just do Wallet first and add that later if we want it. I guess the examples won't be too hard to change (it might even simplify them a bit!).

So here's what Wallet changeset would become:

/// The changes made to a wallet by applying an [`Update`].
#[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize, Default)]
pub struct ChangeSet {
    /// Changes to the [`LocalChain`].
    ///
    /// [`LocalChain`]: local_chain::LocalChain
    pub chain: local_chain::ChangeSet,

    /// Changes to [`IndexedTxGraph`].
    ///
    /// [`IndexedTxGraph`]: bdk_chain::indexed_tx_graph::IndexedTxGraph
    pub indexed_tx_graph: indexed_tx_graph::ChangeSet<
        ConfirmationTimeHeightAnchor,
        descriptor::ChangeSet,
    >,

    /// Stores they keychains of the wallet and their descriptors.
    /// This should only be non-empty in the first changeset of the wallet.
    keychains: BTreeMap<KeychainKind, DescriptorId>.

    /// Stores the network type of the wallet.
    pub network: Option<Network>,   
}

Of course this is non-monotone but it's better to have these hacks up here than down in bdk_chain. Since our API doesn't allow you to mutate descriptors it could also be fixed by #1193 i.e. setting keychains and network in the header. There is a question about whether keychains should have a DescriptorId or a Descriptor in it. If you use DescriptorId you always have to make sure the Descriptor is in the index and have an error case where a DescriptorId gets added to keychains but not indexed_tx_graph. Using the Descriptor in keychains redundantly stores the descriptors but remove the error case. I think I lean to just have DescriptorId since I don't see it causing too much trouble.

We'll probably want to add a descriptor_id() to DescriptorExt in bdk_chain.

Thoughts?

Thoughts?

So after sleeping on it I have changed my view a little bit. The design above is the most in line with the overall design philosophy and removes all possible invalid representations at the bdk_chain level. But it is a very nice feature to label descriptors and we use it in examples and almost every user will want to attach some kind of application semantics to the descriptor. So I want to modify the above proposal to keep KeychainTxOutIndex but have it internally function mostly like what was suggested above so that we remove most of the surface area for footguns.

#[derive(Clone, Debug)]
pub struct KeychainTxOutIndex<K> {
    inner: SpkTxOutIndex<(DescriptorId, u32)>,
    // descriptors of each keychain
    descriptors: BTreeMap<DescriptorId, Descriptor<DescriptorPublicKey>>,
    // last revealed indexes
    last_revealed: BTreeMap<DescriptorId, u32>,
    // lookahead settings for each descriptor
    lookahead: BTreeMap<DescriptorId, u32>,

    keychains: BTreeMap<K, DescriptorId> 
}

#[derive(Clone, Debug, PartialEq, serde::Deserialize, serde::Serialize)]
pub struct ChangeSet<K> {
    revealed: BTreeMap<DescriptorId, u32>,
    keychains_added: BTreeMap<K, Descriptor<DescriptorPublicKey>
}

This means the non-montonicity is restricted to keychains_added field where we can implement keychains_added by just replacing descriptors with the most recent one (i.e. just use BTreeMap::append). Replacing a keychain will not be a permanent break of anything -- You could just re-replace it with the correct one and all the other values like last_revealed would return to their correct values. In fact this even makes it possible to replace a keychain with another descriptor while not losing the old coins. Since the last revealed is associated with the descriptor rather than keychain it will also always load all the old spks. So this turns a bug into a feature in a relatively safe way -- it's probably not a feature that you should ever use though!

@LLFourn Thanks for the detailed explanation. It helped clarify for me why persisting descriptor data at the wallet level is perhaps not enough. I think this direction is well aligned with the related effort of separating out a "header" changeset.

I'll take a crack at implementing your new KeychainTxOutIndex<K> design, but if @danielabrozzoni gets to it first in this PR that's fine too. At least I'll be a better reviewer if I play with it a bit. @danielabrozzoni has this one, I'll still help review.

Please rebase now that #1183 has been merged.

@LLFourn very nice!

I fixed some new CI issues with #1247, it just needs to be ACKd and merged and then this PR rebased.

After a call with @evanlinjin we decided to go with his solution of using Ord by Keychain for methods that return a K (one thing I didn't understand, is that methods that take in a K will continue to work normally).

Then, either of us will open an issue in order to discuss further improvements (I don't like this solution that much and proposed one where we'd return only the latest keychain added, using a Vec instead of a set, but it still needs some discussion).

At the moment, replacing descriptors is still allowed, and having multiple keychains for the same descriptor is still allowed, even when using insert_descriptor.

POST merge ACK. Thanks for just one of the non-perfect options and pushing this one through. Let's see how we feel about it after using it.

I left one comment that I think should be addressed.